rpc: Implement possibility to stop and resume all the RPC communication #1886

MarekPorwisz · 2025-10-21T13:34:58Z

Implemented stop function that immedtaietly releases all threads waiting for RPC response and automatically fails all new calls. Implemented resume function that restores normal RPC communication. This is intended to be used for recovery process when the other side needs to be reset but there are calls waiting for the response. Additionally it is possible to provide callback handler that could be used to do the custom cleanup, for example clear the callback table.

Damian-Nordic · 2025-10-21T13:48:40Z

nrf_rpc/include/nrf_rpc.h

+void nrf_rpc_register_cleanup_handler(struct nrf_rpc_cleanup_handler * handler);
+
+
+/** @brief Temporairly suspend all RPC communication


Suggested change

/** @brief Temporairly suspend all RPC communication

/** @brief Temporarily suspend all RPC communication

Damian-Nordic · 2025-10-21T13:49:17Z

nrf_rpc/include/nrf_rpc.h

+/** @brief Temporairly suspend all RPC communication
+ *
+ * Calling this function automatically fails all communication until @ref nrf_rpc_resume is called.
+ * It also causes all pending request to fail immediately.


Suggested change

* It also causes all pending request to fail immediately.

* It also causes all pending commands to fail immediately.

Damian-Nordic · 2025-10-21T13:52:22Z

nrf_rpc/include/nrf_rpc.h

+ *
+ * @param cleanup set to true to also invoke all the custom cleanup handlers.
+ *
+ * @note If the cleanup parameter is set to true it may be needed to reset the RPC server before invoking


Suggested change

* @note If the cleanup parameter is set to true it may be needed to reset the RPC server before invoking

* @note If the cleanup parameter is set to true it may be needed to reset the peer before invoking

nRF RPC has no concept of client/server.
I think this note may be a bit confusing as to why the peer must be reset though. Also the local device may not be capable of resetting the peer. This note might require some explanation for this reason.

Damian-Nordic · 2025-10-21T13:52:40Z

nrf_rpc/nrf_rpc.c

 #include <stdbool.h>
 #include <string.h>

+#include <zephyr/kernel.h>


No Zephyr includes please.

Damian-Nordic · 2025-10-21T13:56:07Z

nrf_rpc/nrf_rpc.c

+	for (int i = 0; i < CONFIG_NRF_RPC_CMD_CTX_POOL_SIZE; i++) {
+		struct nrf_rpc_cmd_ctx *ctx = &cmd_ctx_pool[i];
+		nrf_rpc_os_mutex_lock(&ctx->mutex);
+		if (ctx->recv_msg.waiting > 0) {


This seems to have changed the nrf_rpc_os_msg - requiring that it support waiting member. If so then nrf_rpc_os_tmpl.h needs to be udpated and release notes added. I wonder if we couldn't rely on ctx->use_count, instead. The only possibility that use_count > 0 and ctx->mutex is unlocked is because the context owners is waiting for a response.

Damian-Nordic · 2025-10-21T14:02:16Z

nrf_rpc/nrf_rpc.c

+	     hdr.type == NRF_RPC_PACKET_TYPE_EVT ||
+	     hdr.type == NRF_RPC_PACKET_TYPE_RSP)) {
+		// drop only selected types of packets
+		// do not drop ACKs INITs and ERRORS


What's the reason for excluding ACKs as well?

I was thinking it is best to allow acks to consider frame delivered and possibly prevent re-transmissions.

ok, but these are not transport-level ACKs. nRF RPC has ACK packets (not frames) to acknowledge event reception :).

Damian-Nordic · 2025-10-21T14:04:05Z

nrf_rpc/nrf_rpc.c

-	hdr.dst_group_id = group->data->dst_group_id;
-	header_cmd_encode(full_packet, &hdr);
+		cmd_ctx = cmd_ctx_reserve();
+		nrf_rpc_os_mutex_unlock(&cleanup_mutex); /* release the mutex as the context specific one has been aqqured */


Suggested change

nrf_rpc_os_mutex_unlock(&cleanup_mutex); /* release the mutex as the context specific one has been aqqured */

nrf_rpc_os_mutex_unlock(&cleanup_mutex); /* release the mutex as the context specific one has been acquired */

kderda · 2025-10-22T12:19:16Z

nrf_rpc/nrf_rpc.c


+static void abort_all_ops(void)
+{
+	NRF_RPC_WRN("Canceling all tasks.");


Should it be a warn? It seems to be expected after user's action, how about using info level?

kderda · 2025-10-22T12:29:16Z

nrf_rpc/include/nrf_rpc.h

+ *                 after the function has finished.
+ *
+ */
+


accidental newline

kderda · 2025-10-22T12:32:13Z

nrf_rpc/include/nrf_rpc.h

+	void (*handler)(void *context);
+
+	/** @brief Custom context passed as the function parameter */
+	void  *context;


Double space. It seems that compliance script is not running for this repository.

kderda · 2025-10-22T12:34:37Z

nrf_rpc/nrf_rpc.c

 	return err;
 }

+void nrf_rpc_register_cleanup_handler(struct nrf_rpc_cleanup_handler * handler)


Suggested change

void nrf_rpc_register_cleanup_handler(struct nrf_rpc_cleanup_handler * handler)

void nrf_rpc_register_cleanup_handler(struct nrf_rpc_cleanup_handler *handler)

kderda · 2025-10-22T12:34:43Z

nrf_rpc/nrf_rpc.c

+{
+	nrf_rpc_os_mutex_lock(&cleanup_mutex);
+
+	struct nrf_rpc_cleanup_handler * current;


Suggested change

struct nrf_rpc_cleanup_handler * current;

struct nrf_rpc_cleanup_handler *current;

kderda · 2025-10-22T12:34:54Z

nrf_rpc/nrf_rpc.c

+	is_stopped = true;
+	abort_all_ops();
+	if (cleanup) {
+		struct nrf_rpc_cleanup_handler * current;


Suggested change

struct nrf_rpc_cleanup_handler * current;

struct nrf_rpc_cleanup_handler *current;

kderda · 2025-10-22T12:35:12Z

nrf_rpc/nrf_rpc.c

+			current->handler(current->context);
+		}
+	}
+	nrf_rpc_os_mutex_unlock(&cleanup_mutex);


nit: maybe newline?

kderda · 2025-10-22T12:38:12Z

nrf_rpc/include/nrf_rpc.h

+ *
+ */
+
+void nrf_rpc_register_cleanup_handler(struct nrf_rpc_cleanup_handler * handler);


Suggested change

void nrf_rpc_register_cleanup_handler(struct nrf_rpc_cleanup_handler * handler);

void nrf_rpc_register_cleanup_handler(struct nrf_rpc_cleanup_handler *handler);

nrf_rpc/include/nrf_rpc.h

Implemented stop function that immedtaietly releases all threads waiting for RPC response and automatically fails all new calls. Implemented resume function that restores normal RPC communication. This is intended to be used for recovery process when the other side needs to be reset but there are calls waiting for the response. Additionally it is possible to provide callback handler that could be used to do the custom cleanup, for example clear the callback table. Signed-off-by: Marek Porwisz <[email protected]>

sonarqubecloud · 2025-10-23T11:32:02Z

Quality Gate passed

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

MarekPorwisz requested review from a team and doki-nordic as code owners October 21, 2025 13:34

NordicBuilder mentioned this pull request Oct 21, 2025

manifest: sdk-nrfxlib: rpc: Implement possibility to stop and resume all the RPC communication nrfconnect/sdk-nrf#25159

Closed

MarekPorwisz mentioned this pull request Oct 21, 2025

samples: nrf_rpc: ps: Support for disabling RPC communication nrfconnect/sdk-nrf#25160

Merged

Damian-Nordic reviewed Oct 21, 2025

View reviewed changes

MarekPorwisz force-pushed the rpc_force_stop branch from e44343f to 547dc8a Compare October 22, 2025 10:23

Damian-Nordic approved these changes Oct 22, 2025

View reviewed changes

MarekPorwisz force-pushed the rpc_force_stop branch from 547dc8a to 7b4557b Compare October 22, 2025 12:08

kderda reviewed Oct 22, 2025

View reviewed changes

MarekPorwisz force-pushed the rpc_force_stop branch from 7b4557b to a8c72d6 Compare October 22, 2025 13:02

RobertGalatNordic reviewed Oct 23, 2025

View reviewed changes

nrf_rpc/include/nrf_rpc.h Show resolved Hide resolved

RobertGalatNordic approved these changes Oct 23, 2025

View reviewed changes

MarekPorwisz force-pushed the rpc_force_stop branch from a8c72d6 to 29c502d Compare October 23, 2025 11:31

kderda approved these changes Oct 23, 2025

View reviewed changes

rlubos merged commit 545ef5d into nrfconnect:main Oct 24, 2025
6 checks passed

		void nrf_rpc_register_cleanup_handler(struct nrf_rpc_cleanup_handler * handler);


		/** @brief Temporairly suspend all RPC communication

	/** @brief Temporairly suspend all RPC communication
	/** @brief Temporarily suspend all RPC communication

	* It also causes all pending request to fail immediately.
	* It also causes all pending commands to fail immediately.

	* @note If the cleanup parameter is set to true it may be needed to reset the RPC server before invoking
	* @note If the cleanup parameter is set to true it may be needed to reset the peer before invoking

	nrf_rpc_os_mutex_unlock(&cleanup_mutex); /* release the mutex as the context specific one has been aqqured */
	nrf_rpc_os_mutex_unlock(&cleanup_mutex); /* release the mutex as the context specific one has been acquired */

	struct nrf_rpc_cleanup_handler * current;
	struct nrf_rpc_cleanup_handler *current;

rpc: Implement possibility to stop and resume all the RPC communication #1886

rpc: Implement possibility to stop and resume all the RPC communication #1886

Uh oh!

Conversation

MarekPorwisz commented Oct 21, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sonarqubecloud bot commented Oct 23, 2025

Quality Gate passed

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants